-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ReferenceTrimmer #5009
base: main
Are you sure you want to change the base?
Use ReferenceTrimmer #5009
Conversation
TODO: Review number of tests + content of nupkg FYI @Youssef1313 |
@@ -19,8 +19,10 @@ | |||
#if ENABLE_CODECOVERAGE | |||
builder.AddCodeCoverageProvider(); | |||
#endif | |||
builder.AddCrashDumpProvider(); | |||
builder.AddAppInsightsTelemetryProvider(); | |||
builder.AddCrashDumpProvider(ignoreIfNotSupported: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why ignoreIfNotSupported was not needed previously
builder.AddHangDumpProvider(); | ||
builder.AddRetryProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Evangelink Do we want to use re-try for unit tests? I'm afraid this could hide race conditions and flaky tests that indicate real issues
@@ -19,8 +19,10 @@ | |||
#if ENABLE_CODECOVERAGE | |||
builder.AddCodeCoverageProvider(); | |||
#endif | |||
builder.AddCrashDumpProvider(); | |||
builder.AddAppInsightsTelemetryProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Evangelink Any reason telemetry wasn't enabled previously? Is it intentional for our tests to opt-out for telemetry? It's already enabled for two other test projects
@@ -20,14 +20,15 @@ | |||
#if ENABLE_CODECOVERAGE | |||
builder.AddCodeCoverageProvider(); | |||
#endif | |||
Console.WriteLine("NATIVE_AOT disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional?
@@ -17,7 +17,7 @@ | |||
<ItemGroup> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Platform\Microsoft.Testing.Platform.csproj" /> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Extensions.TrxReport.Abstractions\Microsoft.Testing.Extensions.TrxReport.Abstractions.csproj" /> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Extensions.Telemetry\Microsoft.Testing.Extensions.Telemetry.csproj" /> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Extensions.Telemetry\Microsoft.Testing.Extensions.Telemetry.csproj" ReferenceOutputAssembly="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this affect the publicly visible dependencies of the VSTestBridge NuGet package?
@@ -37,7 +37,6 @@ This package extends Microsoft Testing Platform to provide a crash dump function | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Platform\Microsoft.Testing.Platform.csproj" /> | |||
<ProjectReference Include="$(RepoRoot)src\Platform\Microsoft.Testing.Extensions.TrxReport.Abstractions\Microsoft.Testing.Extensions.TrxReport.Abstractions.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Does this affect the publicly visible dependencies of CrashDump?
No description provided.